Skip to content

feat(sdk): 59787 merge events#155

Open
namrataghadi-galileo wants to merge 13 commits intomainfrom
feature/59787-merge-events
Open

feat(sdk): 59787 merge events#155
namrataghadi-galileo wants to merge 13 commits intomainfrom
feature/59787-merge-events

Conversation

@namrataghadi-galileo
Copy link
Copy Markdown
Contributor

@namrataghadi-galileo namrataghadi-galileo commented Mar 31, 2026 •
edited
Loading

Summary

  • add an optional SDK-side merged event-creation mode so local and server ControlExecutionEvents can be created together after evaluation
  • keep event creation separate from evaluation by reconstructing events after evaluation results are available, instead of sending full event payloads over the wire
  • reduce wire transfer for the merged path by keeping server evaluation responses focused on evaluation semantics and reconstructing server events in the SDK
  • preserve event identity consistency during reconstruction by using ControlDefinition.observability_identity() for composite conditions
  • keep this PR focused on event creation only by continuing to use the existing built-in SDK/server ingestion path

Behavior

  • default behavior remains unchanged: local events are reconstructed in the SDK and enqueued through the existing SDK observability pipeline, while server-side evaluation still builds and ingests its own events on the server
  • when merge_events=True is enabled for the initialized SDK session, the SDK switches to the merged event-creation path: it reconstructs local and server events after evaluation, sends X-Agent-Control-Merge-Events: true to the server, and enqueues one merged batch through the existing observability pipeline
  • server-side merge mode skips final server ingestion but still returns the same evaluation semantics, so the SDK can merge results and reconstruct the final event batch locally
  • trace/span correlation is preserved in both paths through the tracing resolver and reconstructed control metadata

What changed

  • added shared SDK event reconstruction helpers in evaluation_events.py
  • updated evaluation.py to support:
  • local reconstruction + default enqueue path
  • optional merged event-creation mode gated by initialized session state
  • SDK reconstruction of server events from the lean server response plus cached control definitions
  • fallback to default behavior when the request does not match the active initialized session
  • updated /Users/namrataghadi/code/agentcontrol/agent-control/sdks/python/src/agent_control/init.py and _state.py to add a session-scoped merge_events mode
  • updated evaluation.py so the server:
  • still ingests events in the default path
  • skips final ingestion when X-Agent-Control-Merge-Events: true is set
  • only builds server observability events when observability is enabled

Testing

  • SDK and server tests were updated to cover:
  • default local enqueue behavior
  • merged event-creation behavior
  • provider-backed trace/span propagation
  • fallback when merged mode is requested from the wrong initialized session/client
  • server merge-mode skip-ingest behavior
  • server no-op behavior when observability is disabled

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 31, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 91.15646% with 13 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
sdks/python/src/agent_control/evaluation.py 92.10% 6 Missing ⚠️
sdks/python/src/agent_control/evaluation_events.py 89.79% 5 Missing ⚠️
...r/src/agent_control_server/endpoints/evaluation.py 89.47% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

lan17
lan17 requested changes Apr 1, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@lan17 lan17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are still two blockers before we merge this. I left inline notes on the partial sink integration and the unconditional reconstruction work on the evaluation hot path.

sdks/python/src/agent_control/evaluation.py
)


async def check_evaluation(
Copy link
Copy Markdown
Contributor

@lan17 lan17 Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still leaves the merged-event path only partially integrated across the public SDK surface. If a caller registers set_control_event_sink() but uses the public check_evaluation() helper, we just POST and return the parsed result without setting X-Agent-Control-Merge-Events or reconstructing/emitting any events. I think we need to either wire the sink behavior through this helper too, or explicitly scope the sink API away from this entry point.

Copy link
Copy Markdown
Contributor Author

@namrataghadi-galileo namrataghadi-galileo Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — you’re right. check_evaluation() currently bypasses the merged-event path even when a control event sink is registered, which makes the public SDK surface inconsistent. I think the right fix is to wire the sink behavior through this helper as well: when a sink is present, resolve trace/span IDs, send X-Agent-Control-Merge-Events: true, reconstruct the server events from the lightweight response plus cached control definitions, and emit them through the sink before returning the parsed result. That keeps the default behavior unchanged while making merged-event handling consistent across both public evaluation entry points.

Copy link
Copy Markdown
Contributor Author

@namrataghadi-galileo namrataghadi-galileo Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is no longer valid. I have re-worked this PR to only contain merge model support

sdks/python/src/agent_control/evaluation.py Outdated
local_control_lookup = {
control.id: control.control for control in applicable_local_controls
}
local_events = build_control_execution_events(
Copy link
Copy Markdown
Contributor

@lan17 lan17 Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid reconstructing events unless something will actually consume them? We now always build local events here, and we do the same for server events later, even when merged emission is off and even when OSS observability is disabled. Since evaluation is a latency-sensitive hot path, I don't think we should pay this parsing/allocation cost unless either the existing observability path is active or a merged-event sink is registered.

Copy link
Copy Markdown
Contributor Author

@namrataghadi-galileo namrataghadi-galileo Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure .

Copy link
Copy Markdown
Contributor Author

@namrataghadi-galileo namrataghadi-galileo Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m tightening the hot path so we only reconstruct events when they’ll actually be consumed: local events only when the default SDK observability path is enabled or a sink is registered, and server events only when the merged-event path is active.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abhinav-galileo abhinav-galileo Awaiting requested review from abhinav-galileo

@lan17 lan17 Awaiting requested review from lan17

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@namrataghadi-galileo @lan17